Skip to content

docs(backlog): two follow-ups from the 2026-05-30 merge session#385

Merged
flyingrobots merged 3 commits into
mainfrom
docs/backlog-session-followups
May 31, 2026
Merged

docs(backlog): two follow-ups from the 2026-05-30 merge session#385
flyingrobots merged 3 commits into
mainfrom
docs/backlog-session-followups

Conversation

@flyingrobots
Copy link
Copy Markdown
Owner

@flyingrobots flyingrobots commented May 31, 2026

Summary

Two backlog cards that fell out of patterns I noticed during the PR #382 + #383 + jedit #33 merge session but didn't act on. Both are PLATFORM cleanup; neither blocks Echo 0025 Phase 2.

  • Bad code — `docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md`. The four consumer-crate builder functions in `crates/echo-wesley-gen/tests/generation.rs` each ship ~40 lines of overlapping boilerplate (Cargo.toml, lib.rs shim, no_std/std dep switching). PR chore(dev-loop): kill the slow iteration loop on echo-wesley-gen tests #383 had to touch all four identically when threading `CARGO_TARGET_DIR` through; every future harness change pays the same cost.
  • Cool idea — `docs/method/backlog/cool-ideas/PLATFORM_prettier-before-cargo-hook-ordering.md`. The pre-commit hook still runs the expensive `scripts/verify-local.sh` step before the cheap prettier + markdownlint step. PR chore(dev-loop): kill the slow iteration loop on echo-wesley-gen tests #383's auto-stage fix removed the worst case; this proposes flipping the order so markdown-only commits short-circuit before paying the cold cargo cost.

Test plan

  • `scripts/verify-local.sh pre-commit` clean (took 0s on the docs-only diff — the speedup work paying for itself).
  • `markdownlint-cli2` clean (0 errors across the two new files).
  • Reviewer: read both cards. Land them if the diagnosis matches your read; otherwise close. Neither needs urgency — they are notes for future-self.

Summary by CodeRabbit

  • Documentation
    • Added documentation addressing code duplication patterns and build process optimization strategies.

Bad code: PLATFORM_wesley-gen-test-crate-builder-duplication.md
  Four consumer-crate builder fns in tests/generation.rs each ship
  ~40 lines of overlapping Cargo.toml/lib.rs/dep-declaration
  boilerplate. The PR #383 speedup work had to touch all four
  identically when threading CARGO_TARGET_DIR through; every future
  harness change pays the same cost. Proposes a single
  ConsumerCrateBuilder so a fifth generated-output path is a ~5-line
  call instead of a ~40-line copy.

Cool idea: PLATFORM_prettier-before-cargo-hook-ordering.md
  The pre-commit hook still runs the expensive cargo verify step
  before the cheap prettier + markdownlint step. PR #383's auto-stage
  fix removed the worst case (no more abort+manual-restage), but
  the ordering itself means markdown-only commits still pay the cold
  cargo cost when prettier could have decided the hook can short-
  circuit. Proposes flipping prettier ahead of cargo.

Neither blocks 0025 Phase 2; both are PLATFORM cleanup that pays for
itself the first time anyone touches the same paths again.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 31, 2026

Review Change Stack

Warning

Review limit reached

@flyingrobots, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 27 minutes and 27 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4e487241-774d-4d98-8c71-7434d65149ed

📥 Commits

Reviewing files that changed from the base of the PR and between 7551ccb and 9b97aad.

📒 Files selected for processing (1)
  • docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md
📝 Walkthrough

Walkthrough

Two backlog documentation files added: one identifying duplicated consumer-crate builder boilerplate across wesley-gen tests and proposing centralization; another proposing pre-commit hook reordering to execute fast checks before expensive cargo verification, with risk assessment and expected commit-type deltas.

Changes

Backlog Documentation

Layer / File(s) Summary
Test builder duplication smell
docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md
~40 lines of copy-pasted test builder code across four wesley-gen consumer-crate test functions identified as a code smell. Proposes extraction into a centralized builder (struct or options-based) while maintaining workspace isolation and CARGO_TARGET_DIR behavior.
Pre-commit hook ordering optimization
docs/method/backlog/cool-ideas/PLATFORM_prettier-before-cargo-hook-ordering.md
Proposes moving prettier + markdownlint checks before expensive scripts/verify-local.sh cargo step to improve cold-cache feedback latency. Documents current asymmetry, risk of cargo fmt rewriting markdown-adjacent content and include_str! scenarios, and expected deltas for markdown-only, mixed, and Rust-only commits.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

tooling

Poem

📋 Two notes in the backlog stack,
One spots duplication's knack,
The other speeds the pre-commit race,
With hooks reordered, faster pace.
Both catch the friction, chart the way.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly corresponds to the changeset: two documentation files added to the backlog following a merge session on 2026-05-30, as stated in PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/backlog-session-followups

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md`:
- Around line 54-55: The statement claiming "Not a behavior change" about
removing the PID segment from the crate-dir layout is incorrect; update the
documentation text that describes crate path construction (the "crate-dir layout
(drop the PID segment; use only the label/purpose)" note and the related
sentences around it) to say this is behavior-preserving only if
uniqueness/isolation invariants are maintained (e.g., deterministic unique
labeling or explicit tempdir guarantees); explicitly call out that dropping the
PID changes crate path semantics and may affect test isolation/collision under
parallel runs or label reuse, and apply the same rewording to the other
occurrence covering the same crate path/PID change (the block referenced at the
other occurrence).
- Around line 13-16: Remove the unstable "line ~..." annotations and keep only
stable symbol references: keep the function names write_basic_generated_crate,
write_consumer_smoke_crate, write_contract_host_smoke_crate,
write_optic_binding_smoke_crate (optionally append a permalink/commit hash if
you need a fixed anchor), i.e., edit the list entries to drop the line numbers
and replace them with just the function names (or function name + permalink),
and scan the surrounding text for any other hard-coded source line pointers to
remove or replace similarly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91f8adbc-b185-42ae-b0a1-99d43c3c2f5f

📥 Commits

Reviewing files that changed from the base of the PR and between d3b5ffe and 7551ccb.

📒 Files selected for processing (2)
  • docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md
  • docs/method/backlog/cool-ideas/PLATFORM_prettier-before-cargo-hook-ordering.md

@flyingrobots
Copy link
Copy Markdown
Owner Author

Activity Summary for Code Lawyer pass on PR #385.

# Severity Source File Commit Outcome
1 P4 PR thread docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md 06815ae1 Removed unstable hard-coded source line references and kept stable function-name references.
2 P2 PR thread docs/method/backlog/bad-code/PLATFORM_wesley-gen-test-crate-builder-duplication.md 9b97aadd Qualified the crate-dir PID removal claim: path semantics change is behavior-preserving only if uniqueness/isolation invariants remain guaranteed.

Self-audit found no additional issues beyond the PR review threads.

Local validation performed:

  • Prettier check over both changed PR markdown files: passed.
  • markdownlint over both changed PR markdown files: passed.
  • Pre-push docs-only gate: passed.

All known unresolved review threads have been resolved via GraphQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant